fix: Leading 0 in Token Addresses#310
fix: Leading 0 in Token Addresses#310jonas089 wants to merge 8 commits intoopenintentsframework:mainfrom
Conversation
Add [lib] target to solver-service so it can be used as a library dependency. lib.rs already exists — this just declares it in Cargo.toml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
feat: expose solver-service as a library crate
jonas089
requested review from
NicoMolinaOZ,
nahimterrazas,
pepebndc and
shahnami
as code owners
📝 WalkthroughWalkthroughUpdated address parsing to validate empty inputs and change default hex padding for short/odd-length inputs to 40 chars; added unit tests. Also reorganized workspace members and consolidated/added AWS KMS and mock/testing feature dependencies across multiple Cargo.toml files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can enable review details to help with troubleshooting, context usage and more.Enable the |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/solver-types/src/utils/conversion.rs (1)
147-156:⚠️ Potential issue | 🟡 MinorFix correctly restores leading zeros for truncated address inputs.
The catch-all match arm now pads short inputs (< 40 chars) to 40 characters directly, properly handling addresses that were serialized without leading zeros (e.g., a 39-char hex string from a U256 serialization of an address starting with
0x0...).However, unit tests should be added to cover this edge case and prevent regression. Consider adding tests for:
- 39-char input (the reported bug case)
- Other short inputs (< 40 chars)
- Empty string input
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-types/src/utils/conversion.rs` around lines 147 - 156, Add unit tests for the hex-padding logic in crates/solver-types/src/utils/conversion.rs that exercise the hex_clean -> padded_hex behavior: create tests that pass a 39-char hex string, several other short strings (<40 chars), and an empty string into the conversion function (the module containing the hex_clean/padded_hex logic) and assert the returned padded string matches expected lengths and leading-zero restoration (expect 40 chars for padded 20-byte addresses and 64 chars when code branches to 32-byte padding). Place tests as #[cfg(test)] unit tests near the conversion code or in the crate's tests module and use assert_eq!/assert! checks to ensure no regression for these edge cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/solver-types/src/utils/conversion.rs`:
- Around line 147-156: Add unit tests for the hex-padding logic in
crates/solver-types/src/utils/conversion.rs that exercise the hex_clean ->
padded_hex behavior: create tests that pass a 39-char hex string, several other
short strings (<40 chars), and an empty string into the conversion function (the
module containing the hex_clean/padded_hex logic) and assert the returned padded
string matches expected lengths and leading-zero restoration (expect 40 chars
for padded 20-byte addresses and 64 chars when code branches to 32-byte
padding). Place tests as #[cfg(test)] unit tests near the conversion code or in
the crate's tests module and use assert_eq!/assert! checks to ensure no
regression for these edge cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1089e365-cb5e-46ee-b319-ac7479b781e7
📒 Files selected for processing (1)
crates/solver-types/src/utils/conversion.rs
|
@nahimterrazas I addressed your comment but we need one more approval before I can merge it. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/solver-types/src/utils/conversion.rs (1)
766-774: Consider adding a test for the exact bug scenario (39-char truncated address).The current test validates short input handling with
"0x1", but adding a test case for exactly 39 hex chars would directly document the bug this PR fixes (USDC address starting with0x0...truncated to 39 chars).📝 Suggested additional test
#[test] fn test_parse_address_39_char_truncated_leading_zero() { // Simulates address "0x0abc..." where leading zero was dropped → "0xabc..." (39 chars) // This is the exact bug case: USDC address on EVM chain starting with 0x0... let result = parse_address("0xfbdb2315678afecb367f032d93f642f64180aa3").unwrap(); assert_eq!( hex::encode(&result.0), "0fbdb2315678afecb367f032d93f642f64180aa3" ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-types/src/utils/conversion.rs` around lines 766 - 774, Add a new unit test named test_parse_address_39_char_truncated_leading_zero that calls parse_address with a 39-hex-char input simulating a dropped leading zero (e.g., the USDC case) and assert the returned bytes encode to the correctly padded 40-hex-char address; reference parse_address to locate the parser and ensure the test mirrors the suggested input/expected output for the exact bug scenario.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/solver-types/src/utils/conversion.rs`:
- Around line 766-774: Add a new unit test named
test_parse_address_39_char_truncated_leading_zero that calls parse_address with
a 39-hex-char input simulating a dropped leading zero (e.g., the USDC case) and
assert the returned bytes encode to the correctly padded 40-hex-char address;
reference parse_address to locate the parser and ensure the test mirrors the
suggested input/expected output for the exact bug scenario.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f6bf678a-84cb-4e1c-afd0-6a0344f9b841
📒 Files selected for processing (1)
crates/solver-types/src/utils/conversion.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/solver-storage/Cargo.toml (1)
32-34: Minor:uuidis declared in both[dependencies]and[dev-dependencies].Line 28 already declares
uuid = { workspace = true }in regular dependencies, so the duplicate entry on line 34 in dev-dependencies is redundant. Cargo merges them, so this isn't harmful, but it adds maintenance overhead.♻️ Consider removing the redundant entry
[dev-dependencies] futures = { workspace = true } rust_decimal = { workspace = true } tempfile = { workspace = true } -uuid = { workspace = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/solver-storage/Cargo.toml` around lines 32 - 34, Remove the redundant uuid declaration from the dev-dependencies section: locate the duplicate "uuid = { workspace = true }" entry under [dev-dependencies] and delete it so only the single "uuid = { workspace = true }" in [dependencies] remains, keeping Cargo behavior identical but avoiding maintenance duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/solver-storage/Cargo.toml`:
- Around line 32-34: Remove the redundant uuid declaration from the
dev-dependencies section: locate the duplicate "uuid = { workspace = true }"
entry under [dev-dependencies] and delete it so only the single "uuid = {
workspace = true }" in [dependencies] remains, keeping Cargo behavior identical
but avoiding maintenance duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54e6af50-357d-484b-874b-7b3e97caa517
📒 Files selected for processing (11)
Cargo.tomlcrates/solver-account/Cargo.tomlcrates/solver-core/Cargo.tomlcrates/solver-delivery/Cargo.tomlcrates/solver-demo/Cargo.tomlcrates/solver-order/Cargo.tomlcrates/solver-pricing/Cargo.tomlcrates/solver-service/Cargo.tomlcrates/solver-settlement/Cargo.tomlcrates/solver-storage/Cargo.tomlcrates/solver-types/src/utils/conversion.rs
✅ Files skipped from review due to trivial changes (1)
- crates/solver-demo/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/solver-types/src/utils/conversion.rs
2 participants
This PR fixes a bug where leading 0s in Token addresses are dropped.
I noticed this because the USDC deployment on our EVM chain starts with a "0" e.g. 0x0... and was truncated to 39 digits instead of 40.
This one-line fix should prevent issues like this from happening again in the future.
Summary by CodeRabbit